-
Notifications
You must be signed in to change notification settings - Fork 74
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[RHCLOUD-22813] Handle provided context.host_url in integration templates #3221
base: master
Are you sure you want to change the base?
Conversation
8f61aa6
to
905a9e7
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Awesome work Jessica :)
Because tis PR will involve some braking changes, I think it should be split in two PRs for camel processor case, like:
- Introduce new entries into data source object
- Update Qute templates and remove deprecated data from data source object (like environnment_url)
WDYT?
engine/src/main/java/com/redhat/cloud/notifications/processors/InsightsUrlsBuilder.java
Outdated
Show resolved
Hide resolved
...se/src/main/resources/db/migration/V1.109.0__RHCLOUD-22813_update_template_host_app_urls.sql
Outdated
Show resolved
Hide resolved
engine/src/main/java/com/redhat/cloud/notifications/processors/InsightsUrlsBuilder.java
Outdated
Show resolved
Hide resolved
...se/src/main/resources/db/migration/V1.109.0__RHCLOUD-22813_update_template_host_app_urls.sql
Outdated
Show resolved
Hide resolved
...se/src/main/resources/db/migration/V1.109.0__RHCLOUD-22813_update_template_host_app_urls.sql
Outdated
Show resolved
Hide resolved
engine/src/main/java/com/redhat/cloud/notifications/processors/InsightsUrlsBuilder.java
Outdated
Show resolved
Hide resolved
...y/src/main/java/com/redhat/cloud/notifications/connector/pagerduty/PagerDutyTransformer.java
Show resolved
Hide resolved
engine/src/main/java/com/redhat/cloud/notifications/processors/camel/CamelProcessor.java
Show resolved
Hide resolved
Agree with splitting the PRs. I'll open a new one later today. |
844fc7d
to
7c24129
Compare
...y/src/main/java/com/redhat/cloud/notifications/connector/pagerduty/PagerDutyTransformer.java
Show resolved
Hide resolved
...y/src/main/java/com/redhat/cloud/notifications/connector/pagerduty/PagerDutyTransformer.java
Show resolved
Hide resolved
...uty/src/test/java/com/redhat/cloud/notifications/connector/pagerduty/PagerDutyTestUtils.java
Outdated
Show resolved
Hide resolved
/retest |
@jessicarod7 it looks like those changes breaks test_integration_pagerduty_event_triggered IQE test. |
wasn't expecting this one to fail, looking into it |
engine/src/main/java/com/redhat/cloud/notifications/processors/InsightsUrlsBuilder.java
Outdated
Show resolved
Hide resolved
@g-duval since I can no longer view the Jenkins build (and my attempts to deploy to ephemeral keep timing out), I'm unable to identify why that IQE test is failing |
CamelProcessor tests and db migration not completed yet
8874555
to
99ad7b7
Compare
/retest |
@jessicarod7 , that's right, iqe test execution and logs were moved into Konflux last week, you can check them here. |
Jira issue
https://issues.redhat.com/browse/RHCLOUD-22813
Description
Currently, URLs in PagerDuty and Camel notifications are constructed by the PagerDutyTransformer, or one of three Qute templates, respectively. Each one has a slightly different way of handling URLs, and the Camel templates are not the ones currently shown in this repo.
This PR creates a new class,
InsightsUrlsBuilder
, which provides consistent construction of the two URLs for all integrations:data.inventory_url
represents a link to the specific host, cluster, or inventory item in the Console that generated the message. Ifdata.context.host_url
is provided, it will be used here.data.application_url
links back to the application that generated the event.The PR will also make it easy to modify the URL format in a followup issue.
Database migration
A new migration file (V1.110.0
) has been added. It takes the current templates used in the Notifications database, and updates them to use the new URLs.These changes are now part of #3229.
Testing
CamelProcessorTest
implementers to use the new URLs in their Qute templatesdata.context.host_url
will always be used if presentExample
Using the following trimmed output from
BaseTransformer
:And the new Microsoft Teams template:
We generate this message: